testing: add testing for rewards rate calculation #3336
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3336 +/- ##
==========================================
- Coverage 47.41% 47.40% -0.01%
==========================================
Files 369 369
Lines 59667 59667
==========================================
- Hits 28291 28287 -4
- Misses 28089 28090 +1
- Partials 3287 3290 +3
Continue to review full report at Codecov.
|
|
Looking at it. There is one reviewdog warning if you open the "Files changed" tab. |
| }{ | ||
| // Real values gathered from mainnet | ||
| {"1", 24000000, 215332, 0, 18500000, config.Consensus[protocol.ConsensusCurrentVersion].MinBalance, 6756334087, 18063999}, | ||
| {"2", 24000000, 215332, 545321700, 18500000, 10464550021728, 6756334087, 18063999}, |
There was a problem hiding this comment.
I noticed that NextRewardsState() logs overflow errors. Did you collect all these parameters atomically?
There was a problem hiding this comment.
Under /ledger/internal/eval.go:529 I logged all parameters needed on a node that was sync'd with mainnet.
There was a problem hiding this comment.
Do you have ideas why NextRewardsState() logs overflow errors?
There was a problem hiding this comment.
I don't particularly, they showed up after I changed the round count to be multiplied by 3
There was a problem hiding this comment.
Looks like there is one issue in the test and one issue in the code.
In the test, the parameters {"2", 24000000, 215332, 545321700, 18500000, 10464550021728, 6756334087, are the ones calculated after round 18063999 is processed. However, the first time NextRewardsState() is called, it is given nextRound = 18063999. It should be one higher.
In the code at https://github.com/AlgoStephenAkiki/go-algorand/blob/feature/3284-test-rewards-rate-calculation/data/bookkeeping/block.go#L321 RewardsRate should be taken from res, not s.
There was a problem hiding this comment.
could you examine the block headers from mainnet, find the corresponding rounds and verify the above numbers are correct ?
There was a problem hiding this comment.
I was wrong about the test code issue, the parameters are for given round - 1. However, the rewards residue for test "4" seems wrong.
tsachiherman
changed the title
## Summary A modification of #3336. Added a new test where the rewards pool overspends and proposed a fix in `NextRewardsState()` requiring a consensus upgrade. ## Test Plan This is mostly tests.
|
These changes were already merged in a separate pr. |
* ledger: fix `NextRewardsState()` (#3403) ## Summary A modification of #3336. Added a new test where the rewards pool overspends and proposed a fix in `NextRewardsState()` requiring a consensus upgrade. ## Test Plan This is mostly tests. * Fix a potential problem of committing non-uniform consensus versions (#3453) If accountdb accumulates a large backlog, it is possible catchpoint tracker would attempt to commit a wider range than account updates tracker expects. * avoid generating log error on EnsureValidatedBlock / EnsureBlock (#3424) In EnsureBlock,, do not log as error message if the error is ledgercore.ErrNonSequentialBlockEval and the block round is in the past (i.e. already in the ledger). * Fix typo Fulll to Full (#3456) Fix typo * Fix worng message on restore crash db. (#3455) When crash state is found but could not be restored, noCrashState variable is used to report a warning. However, this variable was set to false in a case where there was no crash state, and the wrong warning was reported. * Adding new scenario for feature networks (#3451) Co-authored-by: Tolik Zinovyev <[email protected]> Co-authored-by: Pavel Zbitskiy <[email protected]> Co-authored-by: Shant Karakashian <[email protected]>
* ledger: fix `NextRewardsState()` (#3403) ## Summary A modification of #3336. Added a new test where the rewards pool overspends and proposed a fix in `NextRewardsState()` requiring a consensus upgrade. ## Test Plan This is mostly tests. * Fix a potential problem of committing non-uniform consensus versions (#3453) If accountdb accumulates a large backlog, it is possible catchpoint tracker would attempt to commit a wider range than account updates tracker expects. * avoid generating log error on EnsureValidatedBlock / EnsureBlock (#3424) In EnsureBlock,, do not log as error message if the error is ledgercore.ErrNonSequentialBlockEval and the block round is in the past (i.e. already in the ledger). * Fix typo Fulll to Full (#3456) Fix typo * Fix worng message on restore crash db. (#3455) When crash state is found but could not be restored, noCrashState variable is used to report a warning. However, this variable was set to false in a case where there was no crash state, and the wrong warning was reported. * Adding new scenario for feature networks (#3451) * Fixing telemetry ports (#3497) Co-authored-by: Tolik Zinovyev <[email protected]> Co-authored-by: Pavel Zbitskiy <[email protected]> Co-authored-by: Shant Karakashian <[email protected]> Co-authored-by: Jack <[email protected]>
Resolves #3284
Adds additional unit tests for the initial block calculation
Summary
Test Plan